Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use safecast for any downcasting #2306

Merged

Conversation

sparrowDom
Copy link
Member

@sparrowDom sparrowDom commented Nov 11, 2024

@sparrowDom sparrowDom changed the base branch from master to sparrowDom/rebaseElsewhere_v2 November 11, 2024 22:14
Copy link

github-actions bot commented Nov 11, 2024

Warnings
⚠️ 👀 This PR needs at least 2 reviewers

Generated by 🚫 dangerJS against 21f7eb7

@@ -277,41 +278,41 @@ contract OUSD is Governable {
returns (int256 rebasingCreditsDiff, int256 nonRebasingSupplyDiff)
{
RebaseOptions state = rebaseState[account];
int256 currentBalance = int256(balanceOf(account));
uint256 newBalance = (currentBalance + balanceChange).toUint256();
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why did this change away from uint256? A balance is a number that can only be positive. It also looks like this changes makes us have to do many more conversions later in the code.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

because of the check on line 283. Though you are right this could be done in a cleaner way. Done here: 21f7eb7

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Good catch.

@DanielVF DanielVF merged commit 0ab7bc8 into sparrowDom/rebaseElsewhere_v2 Nov 13, 2024
9 of 16 checks passed
@DanielVF DanielVF deleted the sparrowDom/rebaseElsewhere_safecast branch November 13, 2024 14:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants